-
Notifications
You must be signed in to change notification settings - Fork 7.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clean constants #50170
Clean constants #50170
Conversation
@@ -175,8 +172,6 @@ const ( | |||
RemoteGatewayClassName = "istio-remote" | |||
WaypointGatewayClassName = "istio-waypoint" | |||
|
|||
// DeprecatedGatewayNameLabel indicates the gateway managing a particular proxy instances. Only populated for Gateway API gateways | |||
DeprecatedGatewayNameLabel = "istio.io/gateway-name" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about the labels of k8s gateways in manifests/charts/istio-control/istio-discovery/files
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove too, as gateway.networking.k8s.io/gateway-name
is also added
} | ||
|
||
// The old label exists on the deployment; use the old label | ||
ti.GatewayNameLabel = constants.DeprecatedGatewayNameLabel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for backwards compatibility from a pretty recent change, not sure we are ready to remove it? @keithmattix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention is to remove in 1.22
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is still a release or 2 too early; it just got merged in 1.20 IIRC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it is marked to be removed in 1.22
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I was probably too aggressive; 1 more release should be ok with an upgrade note
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree with Keith (at the time of the comment being merged, I had worries about 1.22 being too early). Note Keith added the comment.
This code costs us nothing but changing now will break users
e1b4812
to
9e46a77
Compare
…p push when ingress spec not change
9e46a77
to
d5fb49d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to break a lot of ingress stuff, where we don't have a lot of test coverage.
shouldProcess := c.shouldProcessIngressUpdate(ing) | ||
if !shouldProcess { | ||
return nil | ||
} | ||
// only trigger when real changes were found |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem quite right because it doesn't take into account other dependencies? For example, when Service changes, we need to do a push. Even if Ingress doesn't change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don't get why we can remove the AlwaysPush. The VS and GW we are pushing below have no spec, so won't they just always be skipped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as to AlwaysPush
, we have annotation with istio.io
As to This doesn't seem quite right because it doesn't take into account other dependencies?
Thanks for remind, need to revert it. Seems we have no way to distinguish unless updating the input. Let's keep the original
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for annotation := range curr.Meta.Annotations {
if strings.Contains(annotation, "istio.io") {
return true
}
}
``
} | ||
gatewaymetadata := config.Meta{ | ||
Name: item.Name + "-" + "gateway", | ||
Namespace: item.Namespace, | ||
GroupVersionKind: gvk.Gateway, | ||
// Set this label so that we do not compare configs and just push. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this fixed? I don't get it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@howardjohn Take a look at the below logic, not only ingress but also gateway api go through this comparison
func needsPush(prev config.Config, curr config.Config) bool {
...
// If the config is not Istio, let us just push.
if !strings.HasSuffix(prev.GroupVersionKind.Group, "istio.io") {
return true
}
// If current/previous metadata has "*istio.io" label/annotation, just push
for label := range curr.Meta.Labels {
if strings.Contains(label, "istio.io") {
return true
}
}
for annotation := range curr.Meta.Annotations {
if strings.Contains(annotation, "istio.io") { // Here if we checked an annotation, we will always push
return true
}
}
for label := range prev.Meta.Labels {
if strings.Contains(label, "istio.io") {
return true
}
}
for annotation := range prev.Meta.Annotations {
if strings.Contains(annotation, "istio.io") {
return true
}
}
....
return true
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow that's extremely subtle and likely to break in the future. I really prefer the explicit AlwaysPush...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, you know this stull of inner constant looks very hardcoding. I would like to remove all those by updating configHandler
with an explicit arg, so we donot need to compare by needPush
for some resources converted from ingress/gateway-api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But currently we cannot gert rid of InternalRouteSemantics
annotation easily, cause it is used across the code
This reverts commit bef1dd4.
@@ -35,27 +36,11 @@ func needsPush(prev config.Config, curr config.Config) bool { | |||
if !strings.HasSuffix(prev.GroupVersionKind.Group, "istio.io") { | |||
return true | |||
} | |||
// If current/previous metadata has "*istio.io" label/annotation, just push |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was not only for "AlwaysPush". There are annotations that impact XDS generation. Below we only compare spec, so if a config changes just the annotation nothing happens.
Things like istio.io/dry-run, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why should make dryrun push?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any change to inputs that produces a change in outputs should result in a push so things are recomputed. Otherwise the update would be ignored and generate invalid information.
Its not special about "dry-run", just anything where annotation change leads to XDS changes. I recall dry-run was one motivation use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you be more explicit, i donot think of a case we should annotate a config to make istiod push.
Originally this function is to mitigate pushed when labels change to a DR. Simutabeously introduce alwaysPush label, but not sure i know why "*istio.io" should be checked if we checked alwaysPush labels explicitly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We check istio.io only because we know other random annotations are not used in xds generation. It's a hack to improve performance and ignore irrelevant annotations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
..... any example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if val, ok := policy.Annotations[annotation.IoIstioDryRun.Name]; ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just learned another annotation, hha. This makes me hard to do any simplify.
BTW, if we need to keep the feature or this is a useful feature, i think so. we should move it under spec.
@howardjohn another look please |
On Mon, Apr 15, 2024 at 7:54 PM hzxuzhonghu ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pilot/pkg/bootstrap/config_compare.go
<#50170 (comment)>:
> @@ -35,27 +36,11 @@ func needsPush(prev config.Config, curr config.Config) bool {
if !strings.HasSuffix(prev.GroupVersionKind.Group, "istio.io") {
return true
}
- // If current/previous metadata has "*istio.io" label/annotation, just push
I just learned another annotation, hha. This makes me hard to do any
simplify.
BTW, if we need to keep the feature or this is a useful feature, i think
so. we should move it under spec.
We should move EVERYTHING under a spec - as either 'supported for users',
'internal' or 'deprecated',
and add some validation that only 'supported' values can be set by users.
Together with the env variables it creates extreme danger for anyone who
attempts to use OPA or other
validations to restrict users from unsupported features.
… Message ID: ***@***.***>
|
Yes, agree. The path to moving a env/label/annotation to API seems super harder in istio, we can see the number of those stuff is becoming bigger and bigger. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in the other comments, removing this label is too risky and harmful to users; it costs us nothing to keep it longer.
I update the description, only remove |
That's the label I don't want to remove. The motivation for the change seems to be a TODO comment that both the author and approved agree was a mistake. |
🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2024-04-23. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions. Created by the issue and PR lifecycle manager. |
Please provide a description of this PR:
remove
To help us figure out who should review this PR, please put an X in all the areas that this PR affects.
Please check any characteristics that apply to this pull request.